feat[venom]: add loop invariant hoisting pass #4175

HodanPlodky · 2024-07-11T16:07:57Z

What I did

Loop invariant hoisting

How I did it

Implemented loop detection analysis and IR pass which. The detection is done via dfs. The pass detects which instruction are not dependent on instruction within the loop and if not then the instruction is hoisted to the basic block before the loop. This is done iteratively until there are no such instruction in any loop

How to verify it

Commit message

feat[venom]: Loop invariant hoisting

Commit message for the final, squashed PR. (Optional, but reviewers will appreciate it! Please see our commit message style guide for what we would ideally like to see in a commit message.)

Description for the changelog

Cute Animal Picture

harkal · 2024-07-12T08:08:36Z

vyper/venom/analysis/loop_detection.py

+    def analyze(self):
+        self.analyses_cache.request_analysis(CFGAnalysis)
+        self.loops: dict[IRBasicBlock, list[IRBasicBlock]] = dict()
+        done: dict[IRBasicBlock, bool] = dict(


We usually use a set for this purpose, and you will see that we also tent to use our own OrderedSet implementation even when really not visibly necessary, to avoid ending up with some weird non-deterministic code generation.

harkal · 2024-07-12T08:09:19Z

vyper/venom/analysis/loop_detection.py

+    def invalidate(self):
+        return super().invalidate()
+
+    def dfs(self, bb: IRBasicBlock, done: dict[IRBasicBlock, bool], path: list[IRBasicBlock]):


Couldn't this done be promoted to a class member?

I think it could. Should it be promoted even though it is only used for dfs method and is reset before every run of analyze and is not necessary for consumer of this analysis?

harkal · 2024-07-12T08:12:30Z

vyper/venom/analysis/loop_detection.py

+from vyper.venom.basicblock import IRBasicBlock
+
+
+class LoopDetection(IRAnalysis):


As a convention we end all analysis names in Analysis

charles-cooper · 2024-07-19T01:55:29Z

vyper/venom/analysis/loop_detection.py

+        self.dfs(entry)
+
+    def invalidate(self):
+        return super().invalidate()


no need for this implementation

charles-cooper · 2024-07-19T01:57:07Z

vyper/venom/analysis/loop_detection.py

+    def invalidate(self):
+        return super().invalidate()
+
+    def dfs(self, bb: IRBasicBlock, before: IRBasicBlock = None):


this function does loop detection, why name it dfs?

style: we use the convention foo_r() for recursive functions, and prefix with underscore (i.e. _foo_r()) for "private" functions (those that are not called externally)

charles-cooper · 2024-07-19T01:58:45Z

vyper/venom/analysis/loop_detection.py

+        self.done.add(bb)
+        return
+
+    def collect_path(self, bb_from: IRBasicBlock, bb_to: IRBasicBlock) -> OrderedSet[IRBasicBlock]:


style: _collect_path

charles-cooper · 2024-07-19T01:59:06Z

vyper/venom/analysis/loop_detection.py

+        self.collect_path_inner(bb_from, bb_to, loop, collect_visit)
+        return loop
+
+    def collect_path_inner(


_collect_path_inner()

charles-cooper · 2024-07-19T02:21:50Z

vyper/venom/passes/loop_invariant_hosting.py

+    def _get_hoistable_loop(
+        self, from_bb: IRBasicBlock, loop: OrderedSet[IRBasicBlock]
+    ) -> list[tuple[IRBasicBlock, IRBasicBlock, IRInstruction]]:
+        result: list[tuple[IRBasicBlock, IRBasicBlock, IRInstruction]] = []


these type signatures are hard to read -- either make them less specific (e.g. -> list or -> list[tuple]) or make a small dataclass local to this file to carry around the data structure.

charles-cooper · 2024-07-19T02:22:17Z

vyper/venom/passes/loop_invariant_hosting.py

+                return True
+        return False
+
+    def _can_hoist_instruction_ignore_stores(


leave a comment explaining why this is necessary?

maybe just call this _can_hoist_instruction (or just _can_hoist!), as it doesn't need to be contrasted with a function like _can_hoist_instruction_not_ignoring_stores.

charles-cooper · 2024-07-19T02:32:05Z

vyper/venom/passes/loop_invariant_hosting.py

+        for in_var in instruction.get_input_variables():
+            assert isinstance(in_var, IRVariable)
+            source_ins = self.dfg._dfg_outputs[in_var]
+            if source_ins in bb.instructions:


i think we can just check inst.parent for each instruction here

charles-cooper · 2024-07-19T02:34:11Z

vyper/venom/passes/loop_invariant_hosting.py

+                break
+            # I have this inside the loop because I dont need to
+            # invalidate if you dont hoist anything
+            self.analyses_cache.invalidate_analysis(LivenessAnalysis)


let's bring this outside the loop, and add a flag inside the loop to detect if invalidate_analysis should be called

charles-cooper · 2024-07-19T02:37:14Z

vyper/venom/passes/loop_invariant_hosting.py

+    def run_pass(self):
+        self.analyses_cache.request_analysis(CFGAnalysis)
+        self.dfg = self.analyses_cache.request_analysis(DFGAnalysis)
+        loops = self.analyses_cache.request_analysis(LoopDetectionAnalysis)


loops.loops looks weird, maybe just name this t? or loops_analysis

loops_analysis 👍

charles-cooper · 2024-07-19T02:37:38Z

vyper/venom/passes/loop_invariant_hosting.py

+    Any VOLATILE_INSTRUCTIONS, BB_TERMINATORS CFG_ALTERING_INSTRUCTIONS are ignored
+    """
+
+    from typing import Iterator


no need for local import, please hoist this to top of file

charles-cooper · 2024-07-19T02:37:59Z

vyper/venom/passes/loop_invariant_hosting.py

+def _is_correct_store(instruction: IRInstruction) -> bool:
+    return (
+        instruction.opcode == "store"
+        and len(instruction.operands) == 1


this condition is always true, no?

charles-cooper · 2024-07-19T02:40:37Z

vyper/venom/passes/loop_invariant_hosting.py

+        for in_var in instruction.get_input_variables():
+            assert isinstance(in_var, IRVariable)
+            source_ins = self.dfg._dfg_outputs[in_var]
+            if _is_correct_store(source_ins):


how can this condition ever hold true?

…ipping over the literals)

harkal · 2024-07-22T11:10:56Z

vyper/venom/analysis/loop_detection.py

+            assert before is not None, "Loop must have one basic block before it"
+            loop = self._collect_path(before, bb)
+            in_bb = bb.cfg_in.difference({before})
+            assert len(in_bb) == 1, "Loop must have one input basic block"


Since we are looking for natural loops, we should probably rename the analysis to NaturalLoopDetectionAnalysis

We should also probably not assert if the loop was not natural, just ignore that loop

harkal · 2024-07-22T11:20:44Z

vyper/venom/analysis/loop_detection.py

+                self._dfs_r(neighbour, bb)
+
+        self.done.add(bb)
+        return


harkal · 2024-07-22T11:28:11Z

vyper/venom/passes/loop_invariant_hosting.py

+    )
+
+
+def _is_correct_store(instruction: IRInstruction) -> bool:


This is basically _is_store()

harkal · 2024-07-22T11:30:23Z

vyper/venom/passes/loop_invariant_hosting.py

+    def _can_hoist_instruction_ignore_stores(
+        self, instruction: IRInstruction, loop: OrderedSet[IRBasicBlock]
+    ) -> bool:
+        if _ignore_instruction(instruction) or _is_correct_store(instruction):


_ignore_instruction() includes testing for store as it returns True for volatiles. Just an _ignore_instructions() is better.

In the volatiles are only sstore, istore, tstore and mstore, not store. because of that I thought the check for store is necessary or did I miss something?

oh snap... you are right. I had that one named set initially, never got over renaming it :D.. let's just tack that test in the _ignore_instruction()?

Ok I will add it there

charles-cooper · 2024-07-23T03:33:20Z

vyper/venom/passes/loop_invariant_hosting.py

+                return False
+        return True
+
+    def _dependant_in_bb(self, instruction: IRInstruction, bb: IRBasicBlock):


sp: dependent

i think this can be a free function rather than a method, as i don't see any references to self in the body

The DFGAnalysis from self

charles-cooper · 2024-07-23T03:37:18Z

vyper/venom/passes/loop_invariant_hosting.py

+    def _dependant_in_bb(self, instruction: IRInstruction, bb: IRBasicBlock):
+        for in_var in instruction.get_input_variables():
+            assert isinstance(in_var, IRVariable)
+            source_ins = self.dfg._dfg_outputs[in_var]


prefer instruction to always be abbreviated as inst, and also to use the public interface of the dfg analysis, for example:

prev_inst = self.dfg.get_producing_instruction(in_var) # can add `assert prev_inst is not None` if desired

charles-cooper · 2024-07-23T03:38:08Z

vyper/venom/passes/loop_invariant_hosting.py

+        self.analyses_cache.request_analysis(CFGAnalysis)
+        self.dfg = self.analyses_cache.request_analysis(DFGAnalysis)
+        loops = self.analyses_cache.request_analysis(NaturalLoopDetectionAnalysis)
+        self.loop_analysis = loops.loops


it's not an analysis anymore so maybe it should just be self.loops = loops.loops.

charles-cooper · 2024-07-23T03:39:04Z

vyper/venom/analysis/loop_detection.py

+
+    def _collect_path_r(
+        self,
+        act_bb: IRBasicBlock,


does "act_bb" stand for "active bb"? maybe cur_bb or bb_from or just bb would be better.

charles-cooper · 2024-07-23T03:39:27Z

vyper/venom/analysis/loop_detection.py

+        act_bb: IRBasicBlock,
+        bb_to: IRBasicBlock,
+        loop: OrderedSet[IRBasicBlock],
+        collect_visit: OrderedSet[IRBasicBlock],


maybe just visited

charles-cooper · 2024-07-23T03:40:25Z

vyper/venom/passes/loop_invariant_hosting.py

+        self.dfg = self.analyses_cache.request_analysis(DFGAnalysis)
+        loops = self.analyses_cache.request_analysis(NaturalLoopDetectionAnalysis)
+        self.loop_analysis = loops.loops
+        invalidate_dependant = False


dependent what? maybe rename to just invalidate

charles-cooper · 2024-07-23T03:41:01Z

vyper/venom/passes/loop_invariant_hosting.py

+        for inst in hoistable:
+            bb = inst.parent
+            bb.remove_instruction(inst)
+            target_bb.insert_instruction(inst, index=len(target_bb.instructions) - 1)


maybe index does not need to be specified in this case

I am modifying already terminated basic block so I it would crash on assert if the index would not be specified

charles-cooper · 2024-07-23T10:11:56Z

vyper/venom/analysis/loop_detection.py

+            if before is None:
+                return
+            loop = self._collect_path(before, bb)
+            in_bb = bb.cfg_in.difference({before})


i think this might be clearer as if before in bb.cfg_in: .... does that introduce any bugs?

This is just because it checks for natural loops, so I check if it has only one input into the loop.

charles-cooper · 2024-07-23T10:13:14Z

vyper/venom/passes/loop_invariant_hosting.py

+        for var in inst.get_input_variables():
+            source_inst = self.dfg.get_producing_instruction(var)
+            assert isinstance(source_inst, IRInstruction)
+            if _is_store(source_inst):


reduce nesting:

if not _is_store(...): continue

…sting)

Clean up loop detection

codecov · 2024-08-10T14:53:12Z

Codecov Report

Attention: Patch coverage is 95.93496% with 5 lines in your changes missing coverage. Please review.

Project coverage is 88.95%. Comparing base (5067b86) to head (788bd0d).
Report is 15 commits behind head on master.

Files	Patch %	Lines
vyper/venom/passes/loop_invariant_hosting.py	95.00%	3 Missing and 1 partial ⚠️
vyper/venom/analysis/loop_detection.py	97.56%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #4175      +/-   ##
==========================================
- Coverage   91.34%   88.95%   -2.40%     
==========================================
  Files         109      111       +2     
  Lines       15606    15758     +152     
  Branches     3432     3469      +37     
==========================================
- Hits        14256    14018     -238     
- Misses        920     1232     +312     
- Partials      430      508      +78

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

feat[venom]: add loop invariant hoisting pass

13944eb

harkal reviewed Jul 12, 2024

View reviewed changes

HodanPlodky added 3 commits July 12, 2024 14:48

feat[venom]: add unit test for loop invariant hoisting

f626b68

feat[venom]: add unit test for loop invariant hoisting

942c731

feat[venom]: clean up and comments

399a0a4

HodanPlodky marked this pull request as ready for review July 15, 2024 11:39

HodanPlodky added 4 commits July 15, 2024 19:27

fix[venom]: incorrect loop detection fix

2a8dd4a

fix[venom]: removed hoisting just constants

302aa21

fix[venom]: removed unnecessary hoisting

013840c

feat[venom]: clean up after changes

1ee0068

charles-cooper reviewed Jul 19, 2024

View reviewed changes

HodanPlodky added 2 commits July 19, 2024 17:16

fix[venom]: changes from review and better loophoisting structure (sk…

f703408

…ipping over the literals)

fix[venom]: ignore code offset calculation

c5dbf05

harkal reviewed Jul 22, 2024

View reviewed changes

HodanPlodky added 2 commits July 22, 2024 15:32

fix[venom]: changes according to review

cd655fc

fix[venom]: changes according to review

ecb272a

charles-cooper reviewed Jul 23, 2024

View reviewed changes

HodanPlodky and others added 4 commits July 23, 2024 12:28

fix[venom]: changes according to review

18c7610

fix[venom]: review changes (_is_store condition negation to reduce ne…

5583cc5

…sting)

clean up loop detection

bdb2896

Merge pull request #1 from harkal/update_loop_detection

788bd0d

Clean up loop detection

HodanPlodky and others added 3 commits August 12, 2024 10:15

fix[venom]: improved tests to increase coverage

715b128

fix[venom]: error when the store is not constant

8edce11

Merge branch 'master' into loop_invariant

cf6b25e

		from vyper.venom.basicblock import IRBasicBlock


		class LoopDetection(IRAnalysis):

feat[venom]: add loop invariant hoisting pass #4175

Are you sure you want to change the base?

feat[venom]: add loop invariant hoisting pass #4175

Conversation

HodanPlodky commented Jul 11, 2024 • edited Loading

What I did

How I did it

How to verify it

Commit message

Description for the changelog

Cute Animal Picture

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

charles-cooper Jul 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Aug 10, 2024 • edited Loading

Codecov Report

HodanPlodky commented Jul 11, 2024 •

edited

Loading

charles-cooper Jul 23, 2024 •

edited

Loading

codecov bot commented Aug 10, 2024 •

edited

Loading